filesystem: convert free functions to object methods#5692
filesystem: convert free functions to object methods#5692mattklein123 merged 4 commits intoenvoyproxy:masterfrom greenhouse-org:filesystem-object
Conversation
Move the free functions in Envoy::Filesystem to the Filesystem::Instance object. Pass the API object containing the Instance object through everywhere filesystem access is needed. This is step 1/3 in adding Windows filesystem support Signed-off-by: Sophie Wigmore <swigmore@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
This is epic but fairly easy to review with patience. Just a few nits.
| */ | ||
| class Watcher { | ||
| public: | ||
| typedef std::function<void(uint32_t events)> OnChangedCb; |
There was a problem hiding this comment.
Prefer 'using' to 'typedef'.
|
|
||
| class ConfigSchemasTest : public ::testing::TestWithParam<std::string> {}; | ||
| class ConfigSchemasTest : public ::testing::TestWithParam<std::string> { | ||
| public: |
There was a problem hiding this comment.
general pattern is to use 'protected' here in a test class.
test/common/json/json_loader_test.cc
Outdated
| TEST(JsonLoaderTest, Basic) { | ||
| EXPECT_THROW(Factory::loadFromFile("bad_file"), Exception); | ||
| class JsonLoaderTest : public testing::Test { | ||
| public: |
There was a problem hiding this comment.
protected, here & below...I won't point out additional instances of that.
|
and you'll need to merge master of course. |
Signed-off-by: Sam Smith <sesmith177@gmail.com>
|
@sesmith177 thanks for slogging through this. This is going to be great for us in the future. Can you merge master and we can get this merged? |
Signed-off-by: Sam Smith <sesmith177@gmail.com>
|
@mattklein123 @jmarantz merged |
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
@envoyproxy/web-maintainers we should try to merge this as soon as it passes CI and preferably before other PRs get merged, as this is likely to be on an always-needing-merge-then-CI-and-approve treadmill.
|
I think you meant to tag @envoyproxy/maintainers . @mattklein123 's response above sounds like approval, but technically we don't have the senior maintainer approval bit set. So if someone with that power can approve we can merge as soon as CI is complete. |
Move the free functions in Envoy::Filesystem to the Filesystem::Instance object. Pass the API object containing the Instance object through everywhere filesystem access is needed. This is step 1/3 in adding Windows filesystem support Signed-off-by: Sophie Wigmore <swigmore@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>
Move the free functions in Envoy::Filesystem to the Filesystem::Instance object. Pass the API object containing the Instance object through everywhere filesystem access is needed. This is step 1/3 in adding Windows filesystem support Signed-off-by: Sophie Wigmore <swigmore@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Fred Douglas <fredlas@google.com>
4 participants
Description:
Move the free functions in Envoy::Filesystem to the Filesystem::Instance object. Pass the API object containing the Instance object through everywhere filesystem access is needed. Update
canonicalPathto returnSysCallStringResultinstead of throwing an exception as requested in feedback on #5470 (comment)This is step 1/3 in breaking up #5470 to add Windows filesystem support as outlined here: #5470 (comment)
This PR also overlaps a bit with #5660, so we should probably get one of them through first
cc @jmarantz @mattklein123 @lizan
Risk Level:
Low, though this change touches many files.
Testing:
bazel build //source/... && bazel test //test/...Docs Changes:
N/A
Release Notes:
N/A